Fix background naming and remote runtime bootstrap#551
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
More reviews will be available in 23 minutes and 26 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (20)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot review but do not make fixes |
|
@copilot review but do not make fixes |
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Greptile Summary
This PR makes two primary changes: (1) it extracts lane-name fallback logic into a new shared module (
laneNameFallback.ts) and wires both the main process and renderer to use deterministic, prompt-derived fallback names instead of timestamp-based ones; and (2) it changes the remote runtime bootstrap to use an explicit socket path (--socket $ADE_HOME/sock/ade.sock) and removes the pre-connectionruntime stopcall.laneNameFallbackmodule (apps/desktop/src/shared/laneNameFallback.ts): extractsderiveDeterministicLaneNameFromPrompt,GENERIC_LANE_FALLBACK_NAME,genericLaneFallbackName, andgenericSuffixFromLaneFallbackName, resolving the duplicate-stopword-set drift risk flagged in previous reviews. BothagentChatService.ts(main) andAgentChatPane.tsx(renderer) now import from the single source of truth.reasoningEffortpropagation:titleReasoningEffortandsummaryReasoningEffortare added toResolvedChatConfig, plumbed throughrunSessionIntelligenceTask, and respected inptyService.tsfor CLI session title and summary generation. The legacyai.chat.autoTitleReasoningEffortfield is migrated intosessionIntelligence.titles.reasoningEfforton load.stopRemoteRuntimeDaemonis removed; the RPC command now always includes--socket <channel-specific-socket>so a freshly uploaded helper CLI doesn't mismatch-recycle the desktop-owned brain.Confidence Score: 5/5
Safe to merge; changes are well-scoped and thoroughly tested. The shared laneNameFallback module resolves the previously flagged stopword drift, and the remote bootstrap socket change has full test coverage across all channel variants.
All core paths (lane-name derivation, remote bootstrap, reasoning-effort plumbing, null-model-clear) have matching test updates. The only open question is timing of Claude's native vs. ADE-generated title for CLI sessions, but the worst outcome is a brief title flicker, not data loss or a broken flow.
apps/desktop/src/main/services/pty/ptyService.ts — the dual-title-generator path for Claude CLI sessions is new and worth watching in production.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[User submits prompt] --> B[AgentChatPane: compute genericSuffix timestamp] B --> C{titleSettings.enabled?} C -- false --> D[createDeterministicAutoLaneName\nprompt + genericSuffix] C -- true / undefined --> E[suggestAutoLaneName\nwith 10s timeout] E --> F{AI model\nsucceeds?} F -- yes --> G[Use AI-generated name] F -- timeout / error --> H[onFallback warning message] H --> D D --> I[fallbackName = slug or\nparallel-task-TIMESTAMP] G --> J[suggestLaneName IPC call\nto main process] I --> J J --> K[agentChatService:\nsuggestLaneNameFromPrompt] K --> L{AI model\nsucceeds?} L -- yes --> M[normalizeSuggestedLaneName] L -- no --> N[uniquePromptFallbackLaneName\npromptFallback vs explicitFallback] N --> O{promptFallback ==\nGENERIC_LANE_FALLBACK_NAME?} O -- no --> P[Return prompt slug] O -- yes --> Q[genericSuffixFromLaneFallbackName\nextract timestamp from fallbackName] Q --> R[genericLaneFallbackName\nparallel-task-TIMESTAMP] M --> S[Final lane name] P --> S R --> SComments Outside Diff (1)
apps/desktop/src/main/services/sync/syncService.test.ts, line 877-897 (link)syncService.tschangeThe expected
createSyncHostServiceMockcall sequence grew from[8787, 8788](one retry) to eight identical8787attempts before8788, anddisposeFirstAttemptexpectations jumped from 1 to 8 — a 8× change in retry count.syncService.tsis not part of this PR's diff. If the sync service's retry behaviour was changed in a prior commit the test is now correctly documenting it, but ifsyncService.tswasn't changed recently this test update may be masking a silent test failure or a hard-coded retry constant that was altered elsewhere. Worth confirming that the actual runtime retry count matches these expectations intentionally.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "Address lane fallback review" | Re-trigger Greptile